Cargo fmt or what ?

clippy lint application

Please try to run the storage migration test:

function testStorageMigration() public {
        SimpleStorage storageContract = new SimpleStorage();

        // Mark the contract as persistent so it migrates
        vm.makePersistent(address(storageContract));

        storageContract.set(42);
        assertEq(storageContract.get(), 42);

        vm.pvm(false);

        assertEq(storageContract.get(), 42);

        storageContract.set(100);
        assertEq(storageContract.get(), 100);

        vm.pvm(true);
        assertEq(storageContract.get(), 100);
    }

I think it will fail without this code.

it indeed does.
However, if we move storage migration code inside select_revive to always migrate code then it does work.

also why do we need makePersistent? its purely a fork related cheatcode

edit:
makePersistent is a discount vm.register

We can put the storage migration code there, but I think we may have the issue with performance. You will need to sync all contracts - you do not have info which slot was modified.
Regarding makePersistent we used the same approach that zksync uses in select vm. They migrate persistent account as I remember. Maybe due to performance?

We can put the storage migration code there, but I think we may have the issue with performance.

i'll post the benchmarks on morpho test-suite, but it's definitely not the bottle neck as it could be visible there

You will need to sync all contracts - you do not have info which slot was modified.

we can use slot.is_cold() or compare the revm's slot value to current slot value before doing a write, but i doubt that will help with performance as we will still hash the key twice both for read and write

I think you won’t see it in this test suite because the migration runs only once at the beginning, and all contracts with slots are migrated then (they are all newly created). The performance impact should become noticeable when you switch between EVMs many times — at that point it will be painful, because you will need to loop over all contract slots again and again.

it will impact fuzz tests when such cheatcode will be present.
I was thinking of having symmetric solution to this what we have with tracer when we are doing state migration from Revive to REVM. In this case we could skip all cheatcodes custom implementation which modify the storage and use the default implementation.

it will impact fuzz tests when such cheatcode will be present.

why would anyone go out their way to switch vm's in a lot of the fuzz tests?

In this case we could skip all cheatcodes custom implementation which modify the storage and use the default implementation.

not quite ass you'll need to migrate state slots for every contract touched during the test each time any cheatcode is called

My point is that it is always a trade-off between doing things incrementally or as one large operation. Following this direction, we could also skip applying slot changes from the tracer in post_exec.
I prefer incremental changes, but it is OK for now.

As it stands now if we remove state change functionality from the strategy and rely on duplicating the state via apply_revm_diff we would do the whole select_pvm thing on every cheatcode call to update the state on pallet_revive side of things

yes it is a problem, I had such PR but due to performance issues I put it on hold. We would need to have just list of changes.